Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding analyzer feedback to design.md for bird watcher concept exercise #2695

Merged

Conversation

manumafe98
Copy link
Contributor

pull request

Related issue: #2670

Reviewer Resources:

Track Policies

@manumafe98 manumafe98 self-assigned this Jan 31, 2024
@manumafe98 manumafe98 added the x:size/small Small amount of work label Jan 31, 2024
@manumafe98
Copy link
Contributor Author

I have a couple of doubts on the reference resolution:

The clone in the method getLastWeek is necessary? Because it's already making the copy in the constructor.

birdsPerDay[birdsPerDay.length - 1] = birdsPerDay[birdsPerDay.length - 1] + 1;

Also I was thinking that this could be simplified to

    public void incrementTodaysCount() {
        birdsPerDay[birdsPerDay.length - 1]++;
    }

But this one is more peaky.

@sanderploegsma
Copy link
Contributor

I have a couple of doubts on the reference resolution:

The clone in the method getLastWeek is necessary? Because it's already making the copy in the constructor.

I think the author added the .clone() here because the array is a reference value, and by returning a reference to the field, the caller could make changes to it:

birdWatcher.getLastWeek()[0] = 0;

Adding the .clone() makes the method return a copy of the array to prevent this type of mis-use.

birdsPerDay[birdsPerDay.length - 1] = birdsPerDay[birdsPerDay.length - 1] + 1;

Also I was thinking that this could be simplified to

    public void incrementTodaysCount() {
        birdsPerDay[birdsPerDay.length - 1]++;
    }

But this one is more peaky.

Yeah I think you're right. The exercise's .meta/config.json indicates that this exercise was forked from the C# track, and they seem to be doing this too: https://github.com/exercism/csharp/blob/main/exercises/concept/bird-watcher/.meta/Exemplar.cs.

@sanderploegsma
Copy link
Contributor

Reading the exercise instructions again, I think that the implementation of getLastWeek() in the exemplar solution is actually wrong!

For comparison purposes, you always keep a copy of last week's counts nearby, which were: 0, 2, 5, 3, 7, 8 and 4. Implement the BirdWatcher.getLastWeek() method that returns last week's counts:

BirdWatcher.getLastWeek();
// => [0, 2, 5, 3, 7, 8, 4]

From this I conclude that the solution must always return those values. The array passed to the constructor contains the values for this week, not last week. So the exemplar solution should implement getLastWeek() similar to the C# track: https://github.com/exercism/csharp/blob/63dd1aa3fe0bd2b59033158f243d096bd556b9f4/exercises/concept/bird-watcher/.meta/Exemplar.cs#L14

Adding celebratory comment
Updating analyzer comments to require the user to implement a for and for-each loop
Updating reference resolution
@@ -7,15 +7,15 @@ public BirdWatcher(int[] birdsPerDay) {
}

public int[] getLastWeek() {
return birdsPerDay.clone();
return new int[] { 0, 2, 5, 3, 7, 8, 4 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 interesting that the tests don't fail after this change. Makes me wonder whether the test suite is complete... But, I guess that is out of scope for this PR. Might be worth looking into in a separate issue.

Copy link

@phoenix-1729 phoenix-1729 Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should the test cases fail?(Can you please elaborate a little bit)
The array which getLastWeek() is returning is the same array which is being passed while creating the object of Birdwatcher.

`
private static final int DAY1 = 0;
private static final int DAY2 = 2;
private static final int DAY3 = 5;
private static final int DAY4 = 3;
private static final int DAY5 = 7;
private static final int DAY6 = 8;
private static final int TODAY = 4;
private BirdWatcher birdWatcher;

private int lastWeek[] = {DAY1, DAY2, DAY3, DAY4, DAY5, DAY6, TODAY};
@BeforeEach
public void setUp() {
    birdWatcher = new BirdWatcher(lastWeek);
}
@Test
@Tag("task:1")
@DisplayName("The getLastWeek method correctly returns last week's counts")
public void itTestGetLastWeek() {
    assertThat(birdWatcher.getLastWeek())
        .containsExactly(DAY1, DAY2, DAY3, DAY4, DAY5, DAY6, TODAY);
}

`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @phoenix-1729 I recommend to ask the questions in the issue itself

Copy link
Contributor

@sanderploegsma sanderploegsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@manumafe98 manumafe98 merged commit 531301b into exercism:main Jan 31, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/small Small amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants